-
Notifications
You must be signed in to change notification settings - Fork 8k
USB Host: integrate class API [1: types] #94504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Make the struct name match the device naming for ease of use, although slightly longer name. Propagate the change to the subsystem, includes, tests and samples. Signed-off-by: Josuah Demangeon <[email protected]>
subsys/usb/host/usbh_device.c
Outdated
|
||
udev->cfg_desc = k_heap_alloc(&usb_device_heap, | ||
cfg_desc.wTotalLength, | ||
cfg_desc.wTotalLength + sizeof(struct usb_desc_header), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the intention? I remember doing the same, but then I dropped it in favor of just using wTotalLength.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was to add a nil
descriptor at the end so that places like this can be sure to never go past the end of the array:
zephyr/subsys/usb/host/usbh_device.c
Line 288 in 11a9096
while ((dhp->bDescriptorType != 0 || dhp->bLength != 0) && (void *)dhp < desc_end) { |
Maybe the code above can be modified instead? Such as testing for (void *)dhp < desc_end
only and not test for dhp->bDescriptorType != 0 || dhp->bLength != 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the concept of nil
descriptor. I think the descriptor sets for classes should just end with NULL pointer, and the check here should be fine with (void *)dhp < desc_end
condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But of course, this needs to detect bLength == 0
and fail (if it is within wTotalLength
) because the data is tainted (coming from external untrusted source).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you I understand the difference now, I am dropping the commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with this to check dhp
before it is de-referenced:
-while ((dhp->bDescriptorType != 0 || dhp->bLength != 0) && (void *)dhp < desc_end) {
+while ((void *)dhp < desc_end && (dhp->bDescriptorType != 0 || dhp->bLength != 0)) {
subsys/usb/host/usbh_host.h
Outdated
* | ||
* @return true if USB host is in enabled, false otherwise | ||
*/ | ||
static inline bool usbd_is_enabled(const struct usbh_context *const uhs_ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/usbd/usbh
However, I am not sure how it could be used. In my approach, I do not explicitly register any class drivers, I simply place them in an iterable section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am removing it then, thank you
subsys/usb/host/usbh_host.h
Outdated
* | ||
* @return true if USB host is in enabled, false otherwise | ||
*/ | ||
static inline bool usbd_is_initialized(const struct usbh_context *const uhs_ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/usbd/usbh
2a7863a
to
4b8314c
Compare
In the while loop parsing descriptors, check that the descriptor is past the end just before dereferencing it to check if it is seemingly valid. Signed-off-by: Josuah Demangeon <[email protected]>
4b8314c
to
4c8c486
Compare
subsys/usb/host/usbh_host.h
Outdated
static inline bool usbh_is_initialized(const struct usbh_context *const uhs_ctx) | ||
{ | ||
return uhs_ctx->status.initialized; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, perhaps I was not clear. I think it would be better to remove this commit from the PR. It is not used anywhere. I will comment on your second PR once this one has been merged.
d78e394
to
d769180
Compare
Hide the mutex lock details inside a wrapper call, like done for the device stack API. This is not used for the per-device lock of the host stack which use a different mutex lock. Signed-off-by: Josuah Demangeon <[email protected]>
Update the USB shell sample to the new syntax. This is based on tests done with native_sim, along with the virtual.conf, virtual.overlay and device_and_host_prj.conf extra config files. Signed-off-by: Josuah Demangeon <[email protected]>
d769180
to
8a40c80
Compare
|
Well it works much better when testing things before pushing... |
struct sys_bitarray *addr_ba; | ||
}; | ||
|
||
#define USBH_CONTROLLER_DEFINE(device_name, uhc_dev) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please consider adding doxygen docs at some point in the future :) thanks!
Downstream:
This is a very small, incremental change to help downstream pull requests use a shared base.
I was thinking of doing several such small pull requests with just a few commits, to progressively integrate the code of other pull requests:
Here is how I tested it:
Then in another terminal, connect to the console, such as using
picocom /dev/pts/0
.